Repository navigation
Conversation
loveth900
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
-
There are a few improvements you can make.
-
When I checked the code in
index.htmlat https://validator.w3.org/, it indicated some errors.
| if (title.value === "" || pages.value === "") { | ||
| alert("Please fill in all fields!"); | ||
| return false; |
There was a problem hiding this comment.
In terms of input validation,
- Are all input properly checked?
- What if a user enters only space characters in the "title" input field?
- What if a users enters
-1or3.1416in the "pages" input field?
There was a problem hiding this comment.
thank you very much for your obsarvation, i have implemented the needed function
| for (let n = rowsNumber - 1; n > 0; n--) { | ||
| table.deleteRow(n); | ||
| } |
There was a problem hiding this comment.
Instead of deleting the table rows one by one, can you think of a more efficient way to remove all rows (except the <th>...</th>) in the table?
| titleCell.innerHTML = book.title; | ||
| authorCell.innerHTML = book.author; |
There was a problem hiding this comment.
With the way the book's title is assigned to an HTML element, a book with a title containing special character sequence such as <i> can possibly ruin the display.
There was a problem hiding this comment.
Can you think of a more consistent way to name the variables representing the two buttons, changeBut and delButton?
| alert(`You've deleted title: ${book.title}`); | ||
| myLibrary.splice(i, 1); | ||
| render(); | ||
| saveToLocalStorage(); // Save updated library to localStorage | ||
| render(); // Re-render the table |
There was a problem hiding this comment.
At the moment when the alert message is displayed, has the book mentioned in the message been deleted yet?
How would you rearrange these statements to make the message truthful?
There was a problem hiding this comment.
@cjyuan i really appreciate your time and effort, and most espacially the details you pay for everthing!! thanks again for the time.
cjyuan
added
Reviewed
loveth900
added
the
Complete
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.